enhance: broadcast with event-based notification #39522

chyezh · 2025-01-22T12:17:20Z

issue: #38399

broadcast message can carry multi resource key now.
implement event-based notification for broadcast messages
broadcast message use broadcast id as a unique identifier in message
broadcasted message on vchannels keep the broadcasted vchannel now.
broadcasted message and broadcast message have a common broadcast header now.

mergify · 2025-01-22T13:11:51Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

mergify · 2025-01-22T13:13:58Z

@chyezh go-sdk check failed, comment rerun go-sdk can trigger the job again.

codecov · 2025-01-22T13:54:51Z

Codecov Report

Attention: Patch coverage is 86.64495% with 123 lines in your changes missing coverage. Please review.

Project coverage is 81.05%. Comparing base (7476eb3) to head (d75e801).
Report is 40 commits behind head on master.

Files with missing lines	Patch %	Lines
...nternal/streamingcoord/server/service/broadcast.go	23.33%	21 Missing and 2 partials ⚠️
pkg/streaming/util/message/message_impl.go	44.44%	11 Missing and 9 partials ⚠️
...al/streamingcoord/client/broadcast/grpc_watcher.go	85.03%	16 Missing and 3 partials ⚠️
...treamingcoord/client/broadcast/watcher_resuming.go	87.25%	11 Missing and 2 partials ⚠️
...amingcoord/server/broadcaster/broadcast_manager.go	92.50%	6 Missing and 3 partials ⚠️
...server/service/broadcast/broadcast_watch_server.go	88.05%	6 Missing and 2 partials ⚠️
pkg/streaming/util/message/broadcast.go	88.52%	6 Missing and 1 partial ⚠️
...ernal/streamingcoord/server/broadcaster/watcher.go	90.32%	6 Missing ⚠️
pkg/log/with_logger.go	44.44%	3 Missing and 2 partials ⚠️
pkg/streaming/util/message/builder.go	66.66%	1 Missing and 3 partials ⚠️
... and 4 more

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #39522       +/-   ##
===========================================
+ Coverage   69.56%   81.05%   +11.49%     
===========================================
  Files         300     1418     +1118     
  Lines       26986   199595   +172609     
===========================================
+ Hits        18773   161788   +143015     
- Misses       8213    32096    +23883     
- Partials        0     5711     +5711

Components	Coverage Δ
Client	`79.50% <ø> (∅)`
Core	`69.58% <ø> (+0.01%)`	⬆️
Go	`82.98% <86.49%> (∅)`

Files with missing lines	Coverage Δ
internal/coordinator/coordclient/registry.go	`91.17% <ø> (ø)`
internal/distributed/streaming/append.go	`74.19% <ø> (ø)`
internal/distributed/streaming/broadcast.go	`100.00% <100.00%> (ø)`
internal/distributed/streaming/streaming.go	`100.00% <ø> (ø)`
internal/distributed/streaming/wal.go	`64.86% <100.00%> (ø)`
internal/metastore/catalog.go	`100.00% <ø> (ø)`
internal/metastore/kv/streamingcoord/kv_catalog.go	`79.31% <100.00%> (ø)`
internal/rootcoord/broadcast_task.go	`9.61% <ø> (ø)`
internal/streamingcoord/client/client.go	`97.01% <100.00%> (ø)`
internal/streamingcoord/client/client_impl.go	`81.81% <100.00%> (ø)`
... and 24 more

... and 1085 files with indirect coverage changes

mergify · 2025-01-22T15:57:11Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

mergify · 2025-01-23T02:20:16Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

- broadcast message can carry multi resource key now. - implement event-based notification for broadcast messages - broadcast message use broadcast id as a unique identifier in message - broadcasted message on vchannels keep the broadcasted vchannel now. - broadcasted message and broadcast message have a common broadcast header now. Signed-off-by: chyezh <[email protected]>

Signed-off-by: chyezh <[email protected]>

mergify · 2025-01-23T03:41:45Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

chyezh · 2025-01-23T05:14:15Z

/run-cpu-e2e

internal/streamingcoord/client/broadcast/broadcast_impl.go

jaime0815 · 2025-02-06T12:15:42Z

internal/streamingcoord/server/broadcaster/broadcast_task.go

+	// We should always save the task after acked.
+	// Even if the task mark as done in memory.
+	// Because the task is set as done in memory before save the recovery info.
+	return b.saveTask(ctx, b.Logger().With(zap.String("ackVChannel", vchannel)))


we should save the task state before updating its value in memory to avoid receiving a completed task state on the watch side if the persistence fails

Got it.
Current behaviour make the watcher see the event done than event undone after the coord recovery.
It's ok because the consumer of broadcast message will acknowledge until success.
I will add a rollback logic if the save operation is failure in next PR to avoid it.

jaime0815 · 2025-02-07T03:13:59Z

fix the unresolved issue in the next PR.

jaime0815 · 2025-02-07T03:14:26Z

/lgtm

sre-ci-robot · 2025-02-07T03:14:27Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: chyezh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jaime0815 · 2025-02-07T03:27:28Z

The implementation of this gRPC streaming watch mechanism is somewhat repetitive, suggesting to create a common watch function module in the future.

issue: #38399 pr: #39522 - broadcast message can carry multi resource key now. - implement event-based notification for broadcast messages - broadcast message use broadcast id as a unique identifier in message - broadcasted message on vchannels keep the broadcasted vchannel now. - broadcasted message and broadcast message have a common broadcast header now. --------- Signed-off-by: chyezh <[email protected]>

sre-ci-robot requested review from czs007 and godchen0212 January 22, 2025 12:17

sre-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines. label Jan 22, 2025

chyezh mentioned this pull request Jan 7, 2025

[Enhancement]: Support Growing Search With Querynode Delegator And Streaming Service #38399

Open

11 tasks

sre-ci-robot added area/internal-api area/test sig/testing test/integration integration test labels Jan 22, 2025

mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Jan 22, 2025

chyezh force-pushed the enhance_broadcast_with_resource_lock branch from cd1b827 to 4877b2d Compare January 22, 2025 15:15

chyezh force-pushed the enhance_broadcast_with_resource_lock branch from 4877b2d to 752ff02 Compare January 23, 2025 01:43

chyezh added 2 commits January 23, 2025 10:21

fix: unittest

d75e801

Signed-off-by: chyezh <[email protected]>

chyezh force-pushed the enhance_broadcast_with_resource_lock branch from 752ff02 to d75e801 Compare January 23, 2025 02:41

mergify bot added the ci-passed label Jan 23, 2025

chyezh added this to the 2.6.0 milestone Jan 23, 2025

chyezh mentioned this pull request Jan 23, 2025

enhance: broadcast with event-based notification #39550

Merged

SimFG reviewed Feb 5, 2025

View reviewed changes

internal/streamingcoord/client/broadcast/broadcast_impl.go Show resolved Hide resolved

jaime0815 reviewed Feb 7, 2025

View reviewed changes

jaime0815 added the approved label Feb 7, 2025

sre-ci-robot assigned jaime0815 Feb 7, 2025

sre-ci-robot added the lgtm label Feb 7, 2025

sre-ci-robot merged commit a9e0e0a into milvus-io:master Feb 7, 2025
20 checks passed

chyezh deleted the enhance_broadcast_with_resource_lock branch February 7, 2025 03:26

chyezh restored the enhance_broadcast_with_resource_lock branch February 7, 2025 03:36

chyezh deleted the enhance_broadcast_with_resource_lock branch February 7, 2025 03:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance: broadcast with event-based notification #39522

enhance: broadcast with event-based notification #39522

chyezh commented Jan 22, 2025

mergify bot commented Jan 22, 2025

mergify bot commented Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

mergify bot commented Jan 22, 2025

mergify bot commented Jan 23, 2025

mergify bot commented Jan 23, 2025

chyezh commented Jan 23, 2025

jaime0815 Feb 6, 2025 •

edited

Loading

chyezh Feb 7, 2025

jaime0815 commented Feb 7, 2025

jaime0815 commented Feb 7, 2025

sre-ci-robot commented Feb 7, 2025

jaime0815 commented Feb 7, 2025

enhance: broadcast with event-based notification #39522

enhance: broadcast with event-based notification #39522

Conversation

chyezh commented Jan 22, 2025

mergify bot commented Jan 22, 2025

mergify bot commented Jan 22, 2025

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

mergify bot commented Jan 22, 2025

mergify bot commented Jan 23, 2025

mergify bot commented Jan 23, 2025

chyezh commented Jan 23, 2025

jaime0815 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

chyezh Feb 7, 2025

Choose a reason for hiding this comment

jaime0815 commented Feb 7, 2025

jaime0815 commented Feb 7, 2025

sre-ci-robot commented Feb 7, 2025

jaime0815 commented Feb 7, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

jaime0815 Feb 6, 2025 •

edited

Loading